-
-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(loader): pass emitFile
to the child compilation (loaderContext.emitFile
)
#177
fix(loader): pass emitFile
to the child compilation (loaderContext.emitFile
)
#177
Conversation
src/loader.js
Outdated
// Collect assets from modules | ||
compilation.modules.forEach((module) => { | ||
if (module.buildInfo && module.buildInfo.assets) { | ||
assets = { ...assets, ...module.buildInfo.assets }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Object.assign(assets, module.buildInfo.assets)
to reduce the GC stress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/index.js
Outdated
if (!Array.isArray(content) && content != null) { | ||
throw new Error( | ||
`Exported value was not extracted as an array: ${JSON.stringify( | ||
content | ||
)}` | ||
); | ||
} | ||
|
||
module.buildInfo = module.buildInfo || { assets: {} }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| {}
is enough since you add assets
just right after this statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah 👍
@piecyk Also please accept CLA, thanks! |
Thanks @ooflorent for review 👏squash and merge! |
Strange |
Please add tests |
Instead of assigning to module.assets use the |
It's also possible to hook into normalModuleLoader to change the |
module
assets
Blunt, but whtat's the current status of this PR ? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need tests
Sorry for delay... thanks @sokra for tips, will try to finish it over the weekend or close it! |
@piecyk -- any update or timeline on this PR? Also there are some merge conflicts. |
@piecyk can you rebase PR? |
9975b5e
to
23ea2cb
Compare
It looks like this is creating a regression when using a manifest plugin (https://github.com/webdeveric/webpack-assets-manifest in my case). |
Also seeing this change. Some assets are completely missing from the manifest, others are still in there but key values have changed. Example manifest entry:
to
|
@Baune8D very strange, looks something wrong in plugin, we use standard webpack api and just allow loaders to emit |
I created webdeveric/webpack-assets-manifest#39 about this, but I am really unsure this is a manifest plugin bug. This change must have done something that changes what is outputed, and it is not documented. |
Yeah, it looks like something wrong in |
I'm looking at this now to see if its something I can update on my end. I am reading When using |
Maybe getting emitted files bit differently will solve the issue webdeveric/webpack-assets-manifest#39 (comment) |
… the dependencies (Lyrkan) This PR was squashed before being merged into the master branch (closes #403). Discussion ---------- Add new Travis jobs to test lowest/highest versions of all the dependencies This PR adds two Travis jobs that allow to test the lowest and highest versions of all the dependencies (including the dev ones since the version ranges are used by the package helper). I wasn't sure about the method to use for the lowest versions since we can't really guess them without calling the npm's registry API. So, if someone has a better idea... :) Note that both jobs currently fail for the following reasons: - Lowest versions: This isn't really an issue since it seems related to Webpack 4 for which we haven't released a version yet. We could increase the minimum version to match the one from the `yarn.lock` file before releasing. - Highest versions: This is caused by the last version of the `mini-css-extract-plugin` (0.4.3) which doesn't seem to work well with the `webpack-manifest-plugin` anymore (see webpack-contrib/mini-css-extract-plugin#177 (comment)). Closes #39. Commits ------- bcabda6 Add some missing 'return' statements 0161000 Remove hardcoded sass-loader version in addPackagesVersionConstraint test 7abea75 Add new Travis jobs to test lowest/highest versions of all the dependencies
This change appears to have broken some assumptions in our webpack plugin, too. Our issue and exploration of the problem is written up here: https://bitbucket.org/atlassianlabs/atlassian-webresource-webpack-plugin/issues/52 After this change, a
Prior to this change, the asset metadata was stored directly on the
The reason for this behaviour change is precisely because the As a consequence, it's not easy to build a map of original filenames -> compiled filenames by iterating through all If anybody has alternative suggestions for building the map of original filenames -> compiled filenames, I'm keen to hear them. |
After reading through @renchap's @piecyk's and @webdeveric's exploration and fix in the webpack-assets-manifest plugin, I did something similar, with the same effect. Instead of iterating through all Modules in the compilation, I instead hooked in to the
This does what I need it to, but as noted, it may be a hack. If there are better thoughts on how to do this better, let me know :) |
Resolves shellscape#167 mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`. See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177
Second attempt. Based on webdeveric/webpack-assets-manifest#40 Resolves shellscape#167 mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`. See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177
mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`. See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177 fixes #167
mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`. See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177 fixes #167
mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`. See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177 fixes #167
mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`. See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177 fixes #167
mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`. See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177 fixes #167
Resolves shellscape#167 mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`. See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177
Second attempt. Based on webdeveric/webpack-assets-manifest#40 Resolves shellscape#167 mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`. See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177
This PR contains a:
Motivation / Use-Case
Looks like
mini-css-extract-plugin
when collecting sources it's replacing module dependencieswith new
CssDependency
as assets are loaded before. IMHO we can collect and add it back to updated module.Use-case for this is when using
hard-source-plugin
and reading from cache can't find assetmzgoddard/hard-source-webpack-plugin#367
Breaking Changes
No breaking changes, all current test's are passing
Additional Info